-
-
Notifications
You must be signed in to change notification settings - Fork 305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code Review #81
Code Review #81
Conversation
- Removed obsolete `new` keywords. - Added types to collection and function variables. - Added `final` and `const` keywords. - Replaced `null` guards with concise `?.` and `??` operators. - Added missing null check on `dispose` for `FadeAnimatedTextKit`. - In `fade.dart`, renamed the `_RotatingTextState` class to `_FadeTextState` to be consistent with the overall pattern and avoid confusion with `_RotatingTextState` in `rotate.dart`. **Warning**: - Removed `onNextBeforePause` from `ColorizeAnimatedTextKit` because it was not referenced.
Codecov Report
@@ Coverage Diff @@
## master #81 +/- ##
==========================================
+ Coverage 63.24% 68.44% +5.20%
==========================================
Files 7 7
Lines 653 561 -92
==========================================
- Hits 413 384 -29
+ Misses 240 177 -63
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@awhitford, I have a few comments otherwise, this PR looks good to me.
…ul Widget name better.
lib/src/fade.dart
Outdated
? _fadeIn.value | ||
: _fadeOut.value, | ||
opacity: | ||
_fadeIn.value != 1.0 ? _fadeIn.value : _fadeOut.value, | ||
child: Text( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@awhitford Make this Text
widget a child of the AnimatedBuilder
and reference it using the child property given in the builder function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
Also, please resolve the merge conflicts. |
… to some AnimatedBuilders, and added const keyword.
@awhitford Great work! Please do a squash merge. |
@awhitford, I have sent you a collaborator invite to this repo. Great work, hope to see more contributions from you. |
@allcontributors add @awhitford for Ideas & Planning and Maintenance. |
I've put up a pull request to add @awhitford! 🎉 |
new
keywords.final
andconst
keywords.null
guards with concise?.
and??
operators.dispose
forFadeAnimatedTextKit
.fade.dart
, renamed the_RotatingTextState
class to_FadeTextState
to be consistent with the overall pattern and avoid confusion with_RotatingTextState
inrotate.dart
.Warning:
onNextBeforePause
fromColorizeAnimatedTextKit
because it was not referenced.Pull Request Process